Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option for Context Specific spacebar behavior addressing issue #925 #1803

Closed

Conversation

brightOrigin
Copy link
Contributor

Added an additional option to the spacebar behavior pref panes as detailed here #925

@HenningJ
Copy link
Contributor

Can one of the admins verify this patch?

@skurfer
Copy link
Member

skurfer commented Mar 24, 2014

Now, watch. After all my negative Nancy talk about this being impossible to get right, I’ll end up loving it and using it all the time. 😛

I’ve only tested for a few minutes, but it seems to work pretty well. Just a couple of initial observations.

  1. “Context Specific” doesn’t really get across how cool this could be. What about something like “Smart” or “Automatic”?
  2. I’m not so sure about the current behavior with the Shift key. Say I have something like ~/Documents/Work/Policies.pdf. When “Work” is selected, ⇧␣ is equivalent to , taking me up one level to “Documents”. But if “Policies.pdf” is selected, there are no children, so ⇧␣ does a slow-motion Quick Look instead. I think you should only test for children when moving right, but test for parent before moving left.
  3. Text files have children, so you see the lines instead of a Quick Look panel. I don’t think that’s what most people would expect or want, but there’s really no provably “correct” behavior. If you agree with me, I’m not sure exactly how you could work around it. Just excluding files that conform to public.text seems too specific, but that might ultimately be the answer.

I’ll add this into my build and let you know if I see any other issues. Thanks.

@brightOrigin
Copy link
Contributor Author

“Context Specific” doesn’t really get across how cool this could be. What about something like “Smart” or “Automatic”?

Ok, how about Smart Spacebar

I’m not so sure about the current behavior with the Shift key. Say I have something like ~/Documents/Work/Policies.pdf. When “Work” is selected, ⇧␣ is equivalent to ←, taking me up one level to “Documents”. But if “Policies.pdf” is selected, there are no children, so ⇧␣ does a slow- motion Quick Look instead. I think you should only test for children when moving right, but test for parent before moving left.

Sounds good, I never use shift-space and just lazily copied it from the show child contents :)

Text files have children, so you see the lines instead of a Quick Look panel. I don’t think that’s what most people would expect or want, but there’s really no provably “correct” behavior. If you agree with me, I’m not sure exactly how you could work around it. Just excluding files that conform to public.text seems too specific, but that might ultimately be the answer.

This was also bugging me although it didn't know how to fix it. Any more info on how i would go about implementing this? I tried the QSTextType but It seems that text files have a public.data file type so I have resorted to searching the object name for ".txt". That works but im not sure if .txt is the only file extension that QS drills into not to mention that it doesn't seem that clean :)

@skurfer
Copy link
Member

skurfer commented Mar 25, 2014

Ok, how about Smart Spacebar

Better.

Thinking about it more, if the user hits ⇧␣, you probably don’t need to do anything “smart”. Just call moveLeft:. It either works, or it doesn’t, just like with Show Child Contents. Only if they hit Space by itself do you need to check for children, etc.

This was also bugging me although it didn't know how to fix it. Any more info on how i would go about implementing this?

To detect text files in a more flexible way, something like this (untested code) is probably what you want:

QSTypeConformsTo([newSelectedObject fileUTI], @"public.plain-text")

You might have to change newSelectedObject to a QSObject to get fileUTI.

Don’t hesitate to ask about stuff like that. There are a ton of convenience methods all over the place.

@skurfer
Copy link
Member

skurfer commented Mar 25, 2014

Something else that might be nice: When in the second pane, jump to the third. Again, this is untested, but I think this will do it:

if (self == [self actionSelector]) {
    [self shortCircuit:sender];
}

That forces it to select a different action if the current one doesn’t have a third pane, which might be a little surprising at first. But since Space wouldn’t do anything else useful in that context, I think it’s fine. If you were happy with the current action, you’d just hit , right?

@brightOrigin
Copy link
Contributor Author

Something else that might be nice: When in the second pane, jump to the third. Again, this is untested, but I think this will do it:

Yea i like this idea.

I have added this as well as the other updates

[self togglePreviewPanel:nil];
}
// Jump to Indirect if search url
else if ([newSelectedObject containsType:QSSearchURLType])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this addition, but it feels too specific. I think we’d want this behavior for anything where the default action expects an argument. I was about to test that locally, but I’ll mention it since you seem to be around. So stealing some code from QSInterfaceController, you could change the test to

(action && [action respondsToSelector:@selector(argumentCount)] && [action argumentCount] == 2)

And you’d need to do QSAction *action = [[self actionSelector] objectValue] up above somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and maybe test that before testing for Quick Look?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, which line/case are you referring to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of testing [newSelectedObject containsType:QSSearchURLType], use the test above. (I’ve been running it that way and it seems correct.) And then move that behavior up above the Quick Look behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yea in my setup i don't really come across any dObjects that have more then 1 argument besides search urls. Will plug it in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easy way to test it is to select an AppleScript file that uses the on process text feature.

@skurfer skurfer added this to the 2.0.0 milestone Apr 4, 2014
@skurfer
Copy link
Member

skurfer commented Apr 8, 2014

This works pretty well now. Just some small things:

  • Does it really need the ⌘S shortcut? You won’t even know about it until you’ve already clicked with the mouse, and once a choice is made, you probably aren’t coming back to change it frequently.
  • I’d change the menu choice to just “Smart”. I think that fits better with the existing choices like “Normal”.
  • We should change QSSearchSpaceBarBehavior in QSDefaults.plist to 7.

- removed Cmd-S shortcut
- set Smart as default spacebar behavior
@brightOrigin
Copy link
Contributor Author

Does it really need the ⌘S shortcut? You won’t even know about it until you’ve already clicked with the mouse, and once a choice is made, you probably aren’t coming back to change it frequently.

Yea, not sure how that got in there

Pushed a commit with the other changes

@skurfer
Copy link
Member

skurfer commented Apr 17, 2014

Some other small things:

  • I was trying to Quick Look Latest Download and had to hit Space twice since the first one acted like right-arrow. So maybe we should also check that it’s not a proxy object before calling moveRight. But then, if it can’t be Quick Looked, we probably do want to moveRight so it probably needs to check that it is a proxy object, but can’t be Quick Looked. You might be able to accomplish the same thing without making the test even messier by just changing newSelectedObject to [[super objectValue] resolvedObject].
  • @pjrobertson pointed out to me on another branch that we can just use (__bridge NSString *)kUTTypePlainText instead of hard-coding @"public.plain-text”, so we should probably change that.

…proxy objects

- check if objects primary type is a url or search url instead of checking if it simply contains a url. this prevents other objects who use urls to be able to show children like Chrome bookmark folders. Might cause other issues though?
- use string constant instead of hard coding value
@HenningJ
Copy link
Contributor

Can one of the admins verify this patch?

@skurfer
Copy link
Member

skurfer commented Jul 22, 2014

Here’s an interesting use-case: iTunes tracks. They have children, so that’s what get shown by the spacebar, but Quick Look is probably what you wanted. Any thoughts on how to handle that?

@pjrobertson
Copy link
Member

Any thoughts on how to handle that?

The logic could be (haven’t thought it through much):

if (object is file object type && object is not directory) {
quicklook
}

I think that for the most part, for anything that’s file-like (has a public.item entry in its data dictionary) and isn’t a directory/symbolic link/package etc. we’d want to quick look the file

On 22 Gorff 2014, at 16:34, Rob McBroom notifications@github.com wrote:

Here’s an interesting use-case: iTunes tracks. They have children, so that’s what get shown by the spacebar, but Quick Look is probably what you wanted. Any thoughts on how to handle that?


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

skurfer commented Jul 23, 2014

I was thinking we have a default order in which we check things (which matches the way it works now), but then allow for a new array under QSTypeDefinitions that would override the default order for a specific type. A lot more complicated, but it might be the only way to get it right in all situations.

One problem with your suggestion off the top of my head is applications. Would you rather Quick Look them, or see recent documents?

@pjrobertson
Copy link
Member

True, maybe a more fine-grain approach as you say is required. Under the hood, not one we expose to the user…!

On 23 Gorff 2014, at 13:39, Rob McBroom notifications@github.com wrote:

I was thinking we have a default order in which we check things (which matches the way it works now), but then allow for a new array under QSTypeDefinitions that would override the default order for a specific type. A lot more complicated, but it might be the only way to get it right in all situations.

One problem with your suggestion off the top of my head is applications. Would you rather Quick Look them, or see recent documents?


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

skurfer commented Jul 23, 2014

Right. And I’m thinking these overrides would be rare. (I only know of one place where it’s necessary at the moment.)

@pjrobertson
Copy link
Member

Since v1.2 is out the door, we should probably look into this again.

Should we get it out as it is, and then let user feedback dictate what exceptions they'd like fixed (like Rob's iTunes track case)?

@skurfer
Copy link
Member

skurfer commented Oct 27, 2014

See the mailing list for an answer.

@brightOrigin
Copy link
Contributor Author

+100 Yes, please can we push this out :)

I just upgraded QS for Yosemite and not having this is rough.

On Oct 26, 2014, at 6:25 AM, Patrick Robertson notifications@github.com wrote:

Since v1.2 is out the door, we should probably look into this again.

Should we get it out as it is, and then let user feedback dictate what exceptions they'd like fixed (like Rob's iTunes track case)?


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

skurfer commented Nov 21, 2014

It should merge cleanly with master, so it’s easy to keep using it.

get checkout -b foo master
git merge smarter_searchbar_925

Then build QS and you’ll have a copy of 1.2.0 that includes this feature. (Minus the code-signing, so you’ll have to reprove access to Contacts, etc.)

@brightOrigin
Copy link
Contributor Author

Thx Rob,

I tried running that command but ended up with what seemed like an older version of QS. The search results view was all gray. 

I think its because i had master set to my own fork. So i tried 
get checkout -b foo upstream/master
git merge smarter_searchbar_925

But got some merge conflicts on LaunchViewController and some .project issues :(

Ended just manually copy/pasting the changes back in but it would be great to merge this into the next release.

Thx

Tony

From: Rob McBroom notifications@github.com
Reply: quicksilver/Quicksilver reply@reply.github.com>
Date: November 21, 2014 at 6:16:43 AM
To: quicksilver/Quicksilver quicksilver@noreply.github.com>
Cc: brightOrigin tony@brightorigin.com>
Subject:  Re: [Quicksilver] add option for Context Specific spacebar behavior addressing issue #925 (#1803)

It should merge cleanly with master, so it’s easy to keep using it.

get checkout -b foo master
git merge smarter_searchbar_925

Then build QS and you’ll have a copy of 1.2.0 that includes this feature. (Minus the code-signing, so you’ll have to reprove access to Contacts, etc.)


Reply to this email directly or view it on GitHub.

@brightOrigin
Copy link
Contributor Author

Hey guys, any chance of this PR making it into an upcoming release?

@skurfer
Copy link
Member

skurfer commented Mar 30, 2015

100% chance, but I don’t know about timing. I’m hoping to take a week off just to work on QS this summer.

@skurfer
Copy link
Member

skurfer commented Jul 3, 2015

Merged this by hand. It didn’t close, I think because I didn’t have the last commit, but we can live without that.

@skurfer skurfer closed this Jul 3, 2015
skurfer added a commit that referenced this pull request Jul 3, 2015
@brightOrigin
Copy link
Contributor Author

Cool, does that mean this will make an appearance in the next release?

@skurfer
Copy link
Member

skurfer commented Jul 8, 2015

Yes! Thanks for the contribution.

I added some enhancements that I thought I had a pull request for, but apparently, I never submitted it.

@skurfer
Copy link
Member

skurfer commented Oct 22, 2015

I’m updating the credits and I see that we didn’t add you to the list of contributors, @brightOrigin. First, I’m sorry about that, but I’m going to add you now. How would you like to be listed? Your name isn’t attached to your GitHub profile.

@brightOrigin
Copy link
Contributor Author

Thx Rob.

Full name is Tony Papale.

On Oct 22, 2015, 6:17 AM -0700, Rob McBroomnotifications@github.com, wrote:

I’m updating the credits and I see that we didn’t add you to the list of contributors,@brightOrigin(https://github.com/brightOrigin). First, I’m sorry about that, but I’m going to add you now. How would you like to be listed? Your name isn’t attached to your GitHub profile.


Reply to this email directly orview it on GitHub(#1803 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants